Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: scripts #4291

Merged
merged 9 commits into from
Jul 23, 2024
Merged

fix: scripts #4291

merged 9 commits into from
Jul 23, 2024

Conversation

bobz965
Copy link
Collaborator

@bobz965 bobz965 commented Jul 12, 2024

Pull Request

What type of this PR

Examples of user facing changes:

  • Bug fixes

release 1.12-mc

In the helm upgrade process, if user not set OVN_VERSION_COMPATIBILITY, will be blocked in this while true.

so if user not set OVN_VERSION_COMPATIBILITY, should set it to none.


sudo kubectl exec  -it -n kube-system    kubeovn-dzwl2 -- bash

++ grep -o version_compatibility=
++ ovn-nbctl '--db=tcp:[10.233.60.33]:6641' get nb . options
+ '[' x '!=' x ']'
+ echo 'waiting for ovn nb global option version_compatibility to be set...'
waiting for ovn nb global option version_compatibility to be set...
+ sleep 3
+ true
++ grep -o version_compatibility=
++ ovn-nbctl '--db=tcp:[10.233.60.33]:6641' get nb . options
+ '[' x '!=' x ']'
+ echo 'waiting for ovn nb global option version_compatibility to be set...'
waiting for ovn nb global option version_compatibility to be set...
+ sleep 3
+ true


Which issue(s) this PR fixes

Fixes #(issue-number)

@bobz965 bobz965 marked this pull request as draft July 16, 2024 02:04
@bobz965 bobz965 force-pushed the fix-upgrade branch 4 times, most recently from 556acde to 7fedb70 Compare July 22, 2024 06:50
@bobz965 bobz965 marked this pull request as ready for review July 22, 2024 07:27
@bobz965 bobz965 requested a review from zhangzujian July 22, 2024 07:27
@zhangzujian
Copy link
Member

master 分支和 v1.12 也有这个问题?

@zhangzujian
Copy link
Member

sudo kubectl exec -it -n kube-system kubeovn-dzwl2 -- bash

OVN_VERSION_COMPATIBILITY 是空的时候,会有这个 job 吗?

yamls/kind.yaml.j2 Outdated Show resolved Hide resolved
@bobz965
Copy link
Collaborator Author

bobz965 commented Jul 22, 2024

master 分支和 v1.12 也有这个问题?

应该是的,我看 helm 部署的逻辑是如果没有指定,就没有设置 OVN_VERSION_COMPATIBILITY 这个option。如果没有设置过,当 helm upgrade的时候,就会出现 kubeovn-dzwl2 升级 pod卡在 for 循环,不会进入 completed状态,导致helm upgrade timeout。

@zhangzujian
Copy link
Member

sudo kubectl exec -it -n kube-system kubeovn-dzwl2 -- bash

OVN_VERSION_COMPATIBILITY 是空的时候,会有这个 job 吗?

没有设置 OVN_VERSION_COMPATIBILITY 的时候,会有 kubeovn-xxx 这个 job pod 吗?

@bobz965
Copy link
Collaborator Author

bobz965 commented Jul 22, 2024

sudo kubectl exec -it -n kube-system kubeovn-dzwl2 -- bash

OVN_VERSION_COMPATIBILITY 是空的时候,会有这个 job 吗?

没有设置 OVN_VERSION_COMPATIBILITY 的时候,会有 kubeovn-xxx 这个 job pod 吗?

master 后来有一个 PR(e0bcc2967422bcb5d9b95dc43ee7c666d52e12d9),由于有一些ovn层面的改动,考虑到一些稳定性之类的原因,但是我没有立刻回合。

mc 分支应该很小概率会回合 ovn 层面改动的代码了。

@zhangzujian
Copy link
Member

那这个 PR 应该合并到 mc 分支,脚本 if 语法错误可以合并到 master。

@bobz965
Copy link
Collaborator Author

bobz965 commented Jul 22, 2024

这个版本兼容性的设计目的是为了固定 kube-ovn 和 ovn 的版本对应关系么? 热迁移必须使用 22.12之后的代码,但是 24.03又太新,对于我们来说实际上只有一个版本是可用的。

@bobz965
Copy link
Collaborator Author

bobz965 commented Jul 22, 2024

那这个 PR 应该合并到 mc 分支,脚本 if 语法错误可以合并到 master。

恩恩,你说的对。我后面回合下

不过我感觉目前在 ovs upgrade 脚本的检查中有个代码是允许把这个option设计为空的(为空则break),所以目前这个部署时就设计为空的逻辑应该也是ok的吧?

@zhangzujian
Copy link
Member

这个版本兼容性的设计目的是为了固定 kube-ovn 和 ovn 的版本对应关系么? 热迁移必须使用 22.12之后的代码,但是 24.03又太新,对于我们来说实际上只有一个版本是可用的。

mc 分支由你们自己维护,master 上的一些功能设计如果不适用 mc 分支,需要你们自行修改。

@zhangzujian
Copy link
Member

不过我感觉目前在 ovs upgrade 脚本的检查中有个代码是允许把这个option设计为空的(为空则break),所以目前这个部署时就设计为空的逻辑应该也是ok的吧?

你说的部署指的是第一次安装,还是升级?第一次安装和升级都有可能是空,根据镜像 tag 判断的。

@bobz965
Copy link
Collaborator Author

bobz965 commented Jul 22, 2024

不过我感觉目前在 ovs upgrade 脚本的检查中有个代码是允许把这个option设计为空的(为空则break),所以目前这个部署时就设计为空的逻辑应该也是ok的吧?

你说的部署指的是第一次安装,还是升级?第一次安装和升级都有可能是空,根据镜像 tag 判断的。

image

我们之前安装的时候,还没这个设计。但是已经部署过了,后续走升级,这个option就不存在。

既然在安装的时候有考虑过这个直接设置为空的操作,如果用户没有用这个版本兼容的逻辑,是不是可以直接置空就行了。

@bobz965
Copy link
Collaborator Author

bobz965 commented Jul 22, 2024

我在想,我可能直接回合这个PR就行了,而且这个PR和之前那个版本兼容的那个设计应该不冲突。

@zhangzujian
Copy link
Member

没用的代码就没必要合并了吧

@zhangzujian
Copy link
Member

我在想,我可能直接回合这个PR就行了,而且这个PR和之前那个版本兼容的那个设计应该不冲突。

合并一些没有用的代码,会让其它读代码的人疑惑,不知道为什么要这么做。你觉得呢?

@bobz965
Copy link
Collaborator Author

bobz965 commented Jul 22, 2024

我在想,我可能直接回合这个PR就行了,而且这个PR和之前那个版本兼容的那个设计应该不冲突。

合并一些没有用的代码,会让其它读代码的人疑惑,不知道为什么要这么做。你觉得呢?

我重新看了下代码,在后来的代码补充上,其实这个 OVN_VERSION_COMPATIBILITY 是必须要设置的,否则,升级的时候一定会卡。我补充了个错误提示。

Signed-off-by: bobz965 <[email protected]>
Signed-off-by: bobz965 <[email protected]>
Signed-off-by: bobz965 <[email protected]>
Signed-off-by: bobz965 <[email protected]>
@zhangzujian
Copy link
Member

Please change the PR title.

@bobz965 bobz965 changed the title fix: if user not set OVN_VERSION_COMPATIBILITY, should set it to none fix: scripts Jul 23, 2024
@bobz965
Copy link
Collaborator Author

bobz965 commented Jul 23, 2024

Please change the PR title.

ok

@bobz965 bobz965 merged commit 05984fc into kubeovn:master Jul 23, 2024
61 checks passed
@bobz965 bobz965 deleted the fix-upgrade branch July 23, 2024 09:59
zhangzujian added a commit to zhangzujian/kube-ovn that referenced this pull request Jul 23, 2024
* fix: scripts

---------

Signed-off-by: bobz965 <[email protected]>
Co-authored-by: 张祖建 <[email protected]>
Signed-off-by: zhangzujian <[email protected]>
bobz965 added a commit that referenced this pull request Aug 12, 2024
* fix: scripts

---------

Signed-off-by: bobz965 <[email protected]>
Co-authored-by: 张祖建 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants